Skip to content

Conversation

ShE3py
Copy link
Contributor

@ShE3py ShE3py commented Apr 13, 2024

Follow-up of #118625, see #121697.

fn main() {
    match 'b' {
        y.0.0.1.z().f()? as u32 => {},
    }
}

Before:

error: expected one of `=>`, `@`, `if`, or `|`, found `.`
 --> src/main.rs:3:10
  |
3 |         y.0.0.1.z().f()? as u32 => {},
  |          ^ expected one of `=>`, `@`, `if`, or `|`

After:

error: expected a pattern, found an expression
 --> src/main.rs:3:9
  |
3 |         y.0.0.1.z().f()? as u32 => {},
  |         ^^^^^^^^^^^^^^^^^^^^^^^ arbitrary expressions are not allowed in patterns
  |
help: consider moving the expression to a match arm guard
  |
3 |         val if val == y.0.0.1.z().f()? as u32 => {},
  |         ~~~ +++++++++++++++++++++++++++++++++
help: consider extracting the expression into a `const`
  |
2 +     const VAL: /* Type */ = y.0.0.1.z().f()? as u32;
3 ~     match 'b' {
4 ~         VAL => {},
  |
help: consider wrapping the expression in an inline `const` (requires `#![feature(inline_const_pat)]`)
  |
3 |         const { y.0.0.1.z().f()? as u32 } => {},
  |         +++++++                         +

r? fmease
@rustbot label +A-diagnostics +A-parser +A-patterns +C-enhancement

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-diagnostics Area: Messages for errors, warnings, and lints A-parser Area: The lexing & parsing of Rust source code to an AST A-patterns Relating to patterns and pattern matching C-enhancement Category: An issue proposing an enhancement or a PR with one. labels Apr 13, 2024
@rust-log-analyzer

This comment has been minimized.

@fmease
Copy link
Member

fmease commented Apr 27, 2024

Currently vacationing for two more days, then I'm gonna review this PR.

@ShE3py
Copy link
Contributor Author

ShE3py commented Apr 29, 2024

Squashed the commits, I still have the let expr to do (e.g. let buf[0] = expr // remove this let) and some comments to update.

@bors
Copy link
Collaborator

bors commented Jun 19, 2024

☔ The latest upstream changes (presumably #126678) made this pull request unmergeable. Please resolve the merge conflicts.

@rust-log-analyzer

This comment has been minimized.

@ShE3py ShE3py force-pushed the expr-in-pats-2 branch 4 times, most recently from 1361d1a to 27b59b3 Compare July 7, 2024 12:31
@ShE3py ShE3py marked this pull request as ready for review July 7, 2024 14:04
Copy link
Member

@fmease fmease left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've finished reviewing 3 out of 4 commits. Looking good so far. Gonna look at the 4th one in a sec. Sorry for taking so long ^^'

Comment on lines +411 to +432
else {
// We got a trailing method/operator, but that wasn't an expression.
return None;
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice let-else!

Comment on lines 139 to 143
error: expected a pattern, found an expression
--> $DIR/recover-pat-exprs.rs:58:9
|
LL | x.sqrt() @ .. => (),
| ^^^^^^^^ arbitrary expressions are not allowed in patterns
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, if you'd like to suppress this one, we could pass a param RecoverExprInPat::{Yes, No} to parse_pat…

Comment on lines 115 to 471
error: expected a pattern, found an expression
--> $DIR/recover-pat-exprs.rs:43:10
|
LL | (1 + 2) * 3 => (),
| ^^^^^ arbitrary expressions are not allowed in patterns

error: expected a pattern, found an expression
--> $DIR/recover-pat-exprs.rs:43:9
|
LL | (1 + 2) * 3 => (),
| ^^^^^^^^^^^ arbitrary expressions are not allowed in patterns
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to self: Ah, right. If we were to reparse the tokens as an expr when failing to parse a pat (starting right before the first (), then we would only emit a single error for (1 + 2) * 3 in pat position but we don't follow this approach since that would require creating a snapshot upfront potentially in the happy path very likely incurring a perf cost (it's been a while, trying to remember all the context).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nicely structured test file 👏

@bors
Copy link
Collaborator

bors commented Jul 15, 2024

☔ The latest upstream changes (presumably #127777) made this pull request unmergeable. Please resolve the merge conflicts.

@rust-log-analyzer

This comment has been minimized.

@ShE3py
Copy link
Contributor Author

ShE3py commented Sep 5, 2024

Btw, I will gladly insta-approve a PR that just contains the first two commits (a9fbb22 and f8b3046) which are independent of expr-in-pats :)

I was on vacation and I wanted to move some comments (i.e. 2133219), I can split now if that's desirable.


I’ve reverted the “error reducing” for parse_pat_tuple_or_parens so as not to edit the stashing methods;

fn main() {
    let Some((1 + 2) * 3) = None::<i32>;
}
error: expected a pattern, found an expression
 --> src/main.rs:2:14
  |
2 |     let Some((1 + 2) * 3) = None::<i32>;
  |              ^^^^^^^^^^^ arbitrary expressions are not allowed in patterns

error: expected a pattern, found an expression
 --> src/main.rs:2:15
  |
2 |     let Some((1 + 2) * 3) = None::<i32>;
  |               ^^^^^ arbitrary expressions are not allowed in patterns

It's somewhat verbose, but that can be improved in a follow-up.


I wonder if the match arm guard suggestions should be moved to another PR? Tracking issue #129967 will move the guard from the match arm to the pattern, so it might be best to wait for guard_patterns's implementation's PR to be merged before adding guard suggestions.


@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 5, 2024
/// Called by [`Parser::parse_stmt_without_recovery`], used to add statement-aware subdiagnostics to the errors stashed
/// by [`Parser::maybe_recover_trailing_expr`].
pub(super) fn maybe_augment_stashed_expr_in_pats_with_suggestions(&mut self, stmt: &Stmt) {
if self.dcx().has_errors().is_none() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use has_stashed_diagnostic for a more precise test?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It requires the exact span which I don't have, and there's no

fn has_stashed_diagnostics(self: &DiagCtxtInner) -> bool {
    !self.stashed_diagnostics.is_empty()
}

So it's kinda the best I can do without modifying the stashing API.

@bors
Copy link
Collaborator

bors commented Sep 8, 2024

☔ The latest upstream changes (presumably #130091) made this pull request unmergeable. Please resolve the merge conflicts.

Copy link
Member

@fmease fmease left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tracking issue #129967 will move the guard from the match arm to the pattern, so it might be best to wait for guard_patterns's implementation's PR to be merged before adding guard suggestions.

I see what you're getting at but erm, I'd rather merge this first. This PR is a lot older and will immediately affect diagnostics. We don't know how long it will take for the initial guard-pattern PR to get to a ready state and merged. Also, it shouldn't be that hard to update this code, I think.

I'll bors-approve after a rebase. Thanks a lot for your patience! I took way too long for this

@fmease fmease added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 18, 2024
@ShE3py
Copy link
Contributor Author

ShE3py commented Sep 18, 2024

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 18, 2024
@fmease
Copy link
Member

fmease commented Sep 18, 2024

@bors r+

@bors
Copy link
Collaborator

bors commented Sep 18, 2024

📌 Commit db09345 has been approved by fmease

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 18, 2024
@bors
Copy link
Collaborator

bors commented Sep 19, 2024

⌛ Testing commit db09345 with merge df7f778...

@bors
Copy link
Collaborator

bors commented Sep 19, 2024

☀️ Test successful - checks-actions
Approved by: fmease
Pushing df7f778 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 19, 2024
@bors bors merged commit df7f778 into rust-lang:master Sep 19, 2024
7 checks passed
@rustbot rustbot added this to the 1.83.0 milestone Sep 19, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (df7f778): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results (primary 0.9%, secondary 2.2%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
0.9% [0.9%, 0.9%] 1
Regressions ❌
(secondary)
2.2% [2.1%, 2.2%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.9% [0.9%, 0.9%] 1

Cycles

Results (secondary -2.3%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.3% [-2.4%, -2.2%] 4
All ❌✅ (primary) - - 0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 766.835s -> 767.849s (0.13%)
Artifact size: 341.29 MiB -> 341.34 MiB (0.02%)

@ShE3py ShE3py deleted the expr-in-pats-2 branch May 3, 2025 12:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-parser Area: The lexing & parsing of Rust source code to an AST A-patterns Relating to patterns and pattern matching C-enhancement Category: An issue proposing an enhancement or a PR with one. merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants